Log errors from watched queries - #1068
Conversation
Errors raised while resolving or executing a watched query were only reported on the query state and to error listeners. Neither is inspected by default, so `useQuery` appeared to silently do nothing when the query was invalid. Log the error with the database's logger from `AbstractQueryProcessor`, which covers both the table resolution and query execution paths, and do the same for the `runQueryOnce` path in `useSingleQuery`. The default `onError` handler of `watchWithCallback` logged the error itself. That is now handled by the watched query, so the default handler no longer logs to avoid emitting the same error twice.
🦋 Changeset detectedLatest commit: 41fca45 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
simolus3
left a comment
simolus3
left a comment
There was a problem hiding this comment.
I like the idea of logging errors by default. I still think it should be possible to opt-out of that to reduce noise in cases where we know the user has their own error hooks. That's impossible for hooks, but maybe we can improve that for watched queries.
| } | ||
|
|
||
| if (typeof update.error !== 'undefined') { | ||
| if (update.error) { |
There was a problem hiding this comment.
To avoid duplicate logs when users install their own error handlers, could we skip the log if an onError handler exists through iterateAsyncListenersWithError? That means we'd make the onError parameter nullable in watchWithCallback then.
| error: undefined | ||
| })); | ||
| } catch (error) { | ||
| // Matches the logging done by watched queries, so that `runQueryOnce` failures are just as |
There was a problem hiding this comment.
Can we move the compiledQuery variable out of the try block (but still only assign it in there in case compile() throws)? That way, we could include the generated SQL text for queries if execute fails.
2 participants
Overview
useQuerygives no signal at all when the underlying query fails. The error is stored on the returnederrorfield and dispatched toonErrorlisteners, but nothing is inspected by default, so an invalid query looks like a query that simply never returns rows — the console stays empty, as reported in #834.The deprecated callback API does not have this problem:
watchWithCallbackdefaultsonErrorto a logger call (packages/shared-internals/src/client/BasePowerSyncDatabase.ts:659), so theWatchedQuery/useQuerypath lost that behaviour rather than never having it.Changes
Errors are now logged with the database's
logger, matching the existing'Watched query error handler threw an Error'log in the same file.AbstractQueryProcessor.updateStatelogs whenever an error is set on the state. This is the single funnel every watched-query failure passes through, which matters because the two paths are separate:linkQuery(caught byrunWithReporting) — this is what a missing table hits, sinceresolveTablesrunsEXPLAIN <sql>;onChangecallbacks ofOnChangeQueryProcessorandDifferentialQueryProcessor) — this is where runtime failures such as SQLite I/O errors surface.error: nullis used to clear a previous error, so only truthy errors are logged.useSingleQuerylogs the same way for therunQueryOncepath, which does not go through a query processor.watchWithCallback's defaultonErrorno longer logs. The watched query now does that, so keeping it would print the same error twice for every existing consumer of that API. Consumers passing their ownonErrorare unaffected, other than now also getting the log.On log noise
Queries that are expected to fail will now produce output where they previously produced none. That seemed like the right trade-off given the issue: the error is only emitted once per failure, at
errorlevel, and applications that want to handle failures themselves can supply aloggerwith a higherminLevelor their own implementation when constructing the database.Tests
Three tests in
packages/react/tests/useQuery.test.tsx(run in both normal andStrictMode), one per failure path: table resolution, query execution, andrunQueryOnce. Each asserts that the logged record carries the real underlying error, and that it reachesconsole.errorthrough the default logger — the console being empty is what the issue actually describes.All six fail on current
mainwithexpected 0 to be greater than 0.Fixes #834.